Hextof_workflow_steps #169
Conversation
Pull Request Test Coverage Report for Build 6732977899
💛 - Coveralls |
There was a problem hiding this comment.
I know this is early stage, but let me make some general comments. I have the impression we are starting again to develop parallel structures, because of lack of code overview and discussion.
In particular here, I see that develop code for time conversion, that already exists at other places, and introduce duplicates for config entries with different names. I strongly suggest to avoid this, and keep things to a common logic.
I am off course biased because I placed things there first, and am happy to discuss, maybe we should have a separate short meeting to discuss these general design guides if the back and forth on github is too inefficient.
|
@steinnymir all these codestyle issues etc. are typically fixed if you run pre-commit. I typically do this manually before commiting. You can also run the pylint and mypy commands manually, to spot potentiall issues before pushing |
|
@steinnymir Regarding momentum calibration/distortion correction: I have the feeling we are again developing parallel structures. This is what we wanted to avoid. The simplified distortion correction you propose has its advantages, but we found it to be notsufficient, because the residual magnetic fields we have in our lens introduce distortions that cannot be described that way. I would also strongly suggest to separate distortion correction and momentum calibration into two steps, as implemented currently. You can implement your correction as alternative to the current one, if you like, but then put it at the same place and integrate it there, so we don't develop parallel structures. |
I know, the problem is that when I try install pre-commit it creates conflicts in the repo making it unusable. Maybe I can make an other env specific for that. Do you have any suggestions how to do this correctly? do we have a description of the right steps to follow to achieve this, a bit like the readme installation guide? |
These methods we are porting are a direct transfer from The other issue is the complexity of the current calibration methods, which don't make it easy to just jump in and modify methods. Hopefully the end result is to have a unified method, with features from both worlds working as expected. |
Can you be more specific? I had no problems of these sorts. What python version are you on, and which system (linux/windows)? Are you using poetry, or plain venv? That's the venv I am using (pything 3.8.12): |
Great, just wanted to make sure we are on the same track. |
did you install this with Poetry? what steps did you follow? I'll try install py38, poetry and then pip install pre-commit==2.17.0 |
|
I think the issue with the calibration is still with the correct values for the time of flight. For a TOF length of ~1 m, and a drift energy of ~20-30 eV, you expect TOF values of ~300-350 ns. The data show more 600-800 ns. This just does not fit. |
|
|
I remember we had discussions at FLASH about which was the correct range for the ToF. I don't remember what the conclusion was, but I know we decided to represent ToF in the ~700ns range. The difference is only a factor 2 on the step size I believe, so we could test if this helps the energy calibration. It might fit though that the tof distance results in being about sqrt(2) times larger... |
|
are these obtained changing the tof_binsize? |
|
This uses the very same settings, just leaves more freedom to t0. the photon peak is at 0.2E-7 or so, i.e. very far off. I think one should not expect a perfect agreement to physical values, but take this as an empirical calibration. after all, electrons don't have a constant energy in the instrument. |
|
Correlations are very large, that's why the error bars are so large. Maybe one should fix one of values best, e.g. the d. |
|
Further comments to the Notebook:
|
|
To answer your general comments:
This is correct, I tried with the derivative as it is sharper than the peak, and showed better performance than using a broad noisy peak.
I agree we need more points, this is something we'll bring up during beamtime. But I find the instability of the fit is also related to the simplistic peakfind method, as we already discussed.
worked as intended, where you must explicitly mention the sign of each column.
removed functionality entirely.
those are not intended as good fits, but just help show how bad the calibration performed. I agree with the point about the derivative, but as before, using the maxima of a (not fitted) wide peak is not better.
I'm not sure what you mean, but this might be related to a note I made before, where you must delete the old file, it is a security breach. Ideally you should delete your repo and clone it fresh, as the git history has changed and you risk re-introducing it in the history. I never looked into the save_energy_calibration method, I'll have a look and add it if possible. |
What I mean is this file: Line 2 in 61045d0 This is automatically loaded, and contains the mpes energy calibration saved by the tutorial notebook 2. It was in my case used, because I loaded the created config file as user config rather as config, to be able to overwrite the paths. This, however, made that folder_config file overwrite the energy calibration in the user_config... It's a specific problem of the way I included the config, I realize now. |
|
most new functions should be tested now, and what is not should get an issue and will be dealt with later. I am happy with the state of this PR and would like to merge. @rettigl I need your blessing to do so! :) |
This PR contains the tof corrections required for hextof, prior to energy calibration.